-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't send so many synonyms #296
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
"""Map single CURIE.""" | ||
try: | ||
categories, identifiers = data[curie] | ||
except KeyError: | ||
return [curie] | ||
prefixes = { | ||
prefixes = list(dict.fromkeys( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just to deduplicate? Maybe switch to set()
if that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to deduplicate but also retain the order, which is why I've done this rather than using set()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Can we document this in a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
for _curie in identifiers | ||
if _curie.startswith(prefix) | ||
] | ||
if curies: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this returns curies for the first prefix where curies exist, instead of all curies for all prefixes? If I'm understanding it right, can we document this behavior explicitly in a docstring or comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
8fac2b0
to
a797fd2
Compare
a797fd2
to
59980fe
Compare
No description provided.